Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix circular dependencies by making ofrak_angr and ofrak_capstone optional for ofrak_core tests #420

Merged

Conversation

ANogin
Copy link
Contributor

@ANogin ANogin commented Feb 12, 2024

One sentence summary of this PR (This should go in the CHANGELOG!)
Optmizes the build of the --target=development docker containers for the ofrak_core "forward" test dependencies on ofrak_angr and ofrak_capstone

Link to Related Issue(s)
#419

Please describe the changes in your request.
Split the make develop for ofrak_core into two stages, to have the install process agree with the dependency order.

Anyone you think should look at this, specifically?
@whyitfor

@ANogin
Copy link
Contributor Author

ANogin commented Feb 12, 2024

While this works on master, when merged with #417, then during make develop, there is still a "fight" about the version of angr to install, so I am no longer sure this is an OK way to go for #419, marked as draft for now...

@ANogin ANogin changed the title Move internal "forward" dependencies into requirements-test.txt Fix circular dependencies by splitting make develop in core into two stages. May 29, 2024
@ANogin ANogin marked this pull request as ready for review May 29, 2024 05:22
@ANogin
Copy link
Contributor Author

ANogin commented May 29, 2024

@whyitfor @rbs-jacob does this approach make sense?

@ANogin
Copy link
Contributor Author

ANogin commented May 29, 2024

Specifically, this results in something like

develop:
        $(MAKE) -C ofrak_type STAGE=1 develop
        $(MAKE) -C ofrak_io STAGE=1 develop
        $(MAKE) -C ofrak_patch_maker STAGE=1 develop
        $(MAKE) -C ofrak_core STAGE=1 develop
        $(MAKE) -C ofrak_angr STAGE=1 develop
        $(MAKE) -C ofrak_capstone STAGE=1 develop
        $(MAKE) -C frontend STAGE=1 develop
        $(MAKE) -C ofrak_core STAGE=2 develop

where $(MAKE) -C ofrak_core STAGE=1 develop would $(PIP) install -e . (without circular dependencies), and then $(MAKE) -C ofrak_core STAGE=2 develop would $(PIP) install -e .[docs,test] (with circular dependencies, but they are already installed, so no issue).

Copy link
Contributor

@whyitfor whyitfor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach could work, but it introduces some complexity that I'm not sure is worth it.

Could the same end result be achieved by modifying the ofrak develop target to ensure proper install order?

Something like:

develop:
    $(PIP) install -e .
    $(PIP) install -e .[docs,test]

If a 2-3 line change addresses this issue I'd be in favor of that.

Alternatively, should the develop target by default install all ofrak-related dependencies from the source repository (and not from pypi)? I believe @rbs-jacob has suggested such a solution as being advantageous for development as well

@ANogin
Copy link
Contributor Author

ANogin commented Jul 4, 2024

Could the same end result be achieved by modifying the ofrak develop target to ensure proper install order?

Something like:

develop:
    $(PIP) install -e .
    $(PIP) install -e .[docs,test]

No, if would have to be something like

develop:
     $(PIP) install -e .
     $(MAKE) -C disassemblers/ofrak_capstone develop
     $(MAKE) -C disassemblers/angr develop
     $(PIP) install -e .[docs,test]

Would that be OK?

Alternatively, should the develop target by default install all ofrak-related dependencies from the source repository (and not from pypi)? I believe @rbs-jacob has suggested such a solution as being advantageous for development as well

What would be the approach for that? One that would probably work is:

  • In the top-level Makefile, have make develop simply do pip install -e path1 path2 path3...." for all OFRAK paths before the existing $(MAKE) -C path1 develop; ${MAKE) -C path2 develop; ...`, except that it would require that all OFRAK modules are PIP modules (I think frontend is currently the main exception).

@ANogin ANogin force-pushed the bugfix/circular_dependencies branch from e2c91c8 to dcdacdd Compare July 4, 2024 21:54
@ANogin
Copy link
Contributor Author

ANogin commented Jul 4, 2024

@whyitfor , I implemented an alternative approach where only the make develop target in ofrak_core/Makefile changes - does this work?

P.S. The reson for the other two changes is that:

  • root make image rule is changed because ofrak-core-dev.yml no longer makes sense, as a minimal dev install needs to include capstone and angr (that is, ofrak-angr.yml)
  • importlib-resources moved from setup to requirements so that it is installed in the base.Dockerfile stage (there because my test for this fix working right is that the full make develop suceeeds without needing network access)

@ANogin ANogin requested a review from whyitfor July 4, 2024 21:58
@ANogin ANogin force-pushed the bugfix/circular_dependencies branch from dcdacdd to 986aaa0 Compare July 4, 2024 22:00
Makefile Outdated Show resolved Hide resolved
@whyitfor
Copy link
Contributor

@ANogin, can you pull master into this for the CI tests?

@ANogin
Copy link
Contributor Author

ANogin commented Aug 20, 2024

@whyitfor , note my earlier caveat:

root make image rule is changed because ofrak-core-dev.yml no longer makes sense, as a minimal dev install needs to include capstone and angr (that is, ofrak-angr.yml)

So reverting this change is not the right thing to do.

ANogin and others added 7 commits August 20, 2024 13:49
…edballoonsecurity#470)

* Support `application/vnd.android.package-archive` mime type for APKs

* Add a CHANGELOG entry
…rity#468)

* Fix an issue with parallel tests potentially clashing

* Run pre-commit
…edballoonsecurity#472)

* Use GzipFile python unpacker for speed, fall back on pigz if needed

* Add a changelog entry
)

* Fix Ghidra serializing negative 64-bit addresses

* Lint

* Fix lint

* Fixed pre-commit hook

* Add bb data range for capstone test

* Update changelog

* Update changelog

* Add description of new test cases
* Prevent Ghidra autoanalysis on import

* Update CHANGELOG.md

* Add link to PR in changelog

---------

Co-authored-by: Wyatt <53830972+whyitfor@users.noreply.github.com>
…#478)

Co-authored-by: Dan Pesce <dan@redballoonsecurity.com>
ANogin and others added 8 commits August 20, 2024 13:49
…edballoonsecurity#480)

* Make the log file name configurable and include timestamp by default

* Add a changelog entry

* Fix a help string typo

Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>

---------

Co-authored-by: Jacob Strieb <99368685+rbs-jacob@users.noreply.github.com>
…r` (redballoonsecurity#484)

* Use temporary output directory for function replacement test

* Use pathlib joining for file path
Add SHELL instruction to build_image.py to avoid JSONArgsRecommended warning
@ANogin ANogin changed the title Fix circular dependencies by splitting make develop in core into two stages. Fix circular dependencies by making ofrak_angr and ofrak_capstone optional for ofrak_core tests Aug 20, 2024
@ANogin
Copy link
Contributor Author

ANogin commented Aug 20, 2024

@whyitfor in order for things to still [kind of] work with ofrak-core-dev.yml, I tried an alternative approach, which I think is cleaner - the ofrak_angr and ofrak_capstone are not included as dependencies, and the tests that do rely on them are just skipped if they are not installed. The result is that make -C /ofrak_core test succeeds in e.g. ofrak-angr.yml but in ofrak-core-dev.yml while all tests succeed, the coverage check then fails with FAIL Required function coverage of 100.00% not reached. Total coverage: 99.91% (because of the skipped tests).

@ANogin ANogin requested a review from whyitfor August 20, 2024 23:34
Copy link
Contributor

@whyitfor whyitfor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good incremental compromise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants